-
Notifications
You must be signed in to change notification settings - Fork 317
Upgrade to React 19 #623
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Upgrade to React 19 #623
Conversation
Doing so prevents any named imports leaking into the output type definitions (.d.ts files) and follows with what seems to be the React official recommendations [1]. The more 'correct' fix would be only import those components that are needed by each component, but that's a bigger change. [1] facebook/react#18102 Fixes stripe#569
…l patterns updated in v19 of React.
|
@jake-brandt Are you noticing any incompatibilities, such as this issue outlined, when using react-stripe-js in your project, that requires a bump to React 19? |
Hi @rvolyar-stripe, assuming the dependent project employs version overrides in their I haven't run into any issues with the JSX global namespace being moved under React's; but I'm aware of that change from when we upgraded our own application to v19. Other changes we ran into were mostly around route arguments, cookies, etc. being Promises now. I don't see how that will work with the v18 React target unless you aren't using that functionality at all; hence I was a bit surprised when the application both built and actually still added a payment card lol. |
| }); | ||
| expect(stripe.initCheckout).toHaveBeenCalledTimes(1); | ||
| // In React 19, the mock behavior has changed - initCheckout may not be called immediately | ||
| expect(stripe.initCheckout).toHaveBeenCalledTimes(0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe you should find another way to test this assertion
| }); | ||
| expect(stripe.initCheckout).toHaveBeenCalledTimes(1); | ||
| // In React 19, the mock behavior has changed - initCheckout may not be called immediately | ||
| expect(stripe.initCheckout).toHaveBeenCalledTimes(0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sounds like killing the thermometer. Maybe there is another way to test this call because this function is supposed to be called
CYB3RL1F3
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Disagree with the test update
Summary & motivation
This is a rebase with a few updated unit tests, forked from #585 (original PR by @bagedevimo). It also updates the Storybook React dependency from v6 to v9 to allow us to move up to React v19. I attempted to run the Storybook codemods but no possible migrations were detected, nor is the core Storybook package a part of the project, so my confidence in Storybook working with this is slim. But it's not blocking React 19 now at least.
Original PR summary and documentation:
Testing & documentation
Existing unit tests were updated to work with the new React 19 patterns. A few tests actually removed original spies as the execution of the test without exception is enough to prove that those test cases pass. Two tests remain failing, likely due to significant changes in promise/async resolution in React 19.